Skip to content

GH-50779: [C++][Parquet] Replace remaining RapidJSON usage with simdjson - #50781

Open
Reranko05 wants to merge 6 commits into
apache:mainfrom
Reranko05:gh-35460-pr5-updated
Open

GH-50779: [C++][Parquet] Replace remaining RapidJSON usage with simdjson#50781
Reranko05 wants to merge 6 commits into
apache:mainfrom
Reranko05:gh-35460-pr5-updated

Conversation

@Reranko05

@Reranko05 Reranko05 commented Aug 3, 2026

Copy link
Copy Markdown
Collaborator

Rationale for this change

This PR continues the simdjson migration by replacing the remaining RapidJSON usage under cpp/src/parquet with simdjson and JsonWriter. It also updates the Meson build to support simdjson and removes unnecessary RapidJSON dependencies from the Parquet build configuration.

What changes are included in this PR?

  • Replace the remaining RapidJSON parsing logic in reader_test.cc with simdjson.
  • Replace RapidJSON parsing and serialization in geospatial/util_json_internal.cc with simdjson and JsonWriter.
  • Replace RapidJSON string escaping in types.cc with JsonWriter.
  • Add Meson support for simdjson, including a fallback CMake subproject.
  • Link simdjson in the relevant Meson targets.
  • Remove unnecessary RapidJSON dependencies from the Parquet Meson and CMake build configuration.

Copilot AI lite review requested due to automatic review settings August 3, 2026 10:41
@Reranko05
Reranko05 requested review from pitrou and wgtmac as code owners August 3, 2026 10:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added the awaiting review Awaiting review label Aug 3, 2026
@github-actions

github-actions Bot commented Aug 3, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #50779 has been automatically assigned in GitHub to PR creator.

@Reranko05
Reranko05 marked this pull request as draft August 3, 2026 10:55
@Reranko05 Reranko05 added the CI: Extra: C++ Run extra C++ CI label Aug 3, 2026
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from d026d1f to 0230946 Compare August 3, 2026 12:54
@Reranko05

Copy link
Copy Markdown
Collaborator Author

@rok @taepper

I'm migrating parquet/geospatial/util_json_internal.cc and ran into an ondemand question.

GeospatialGeoArrowCrsToParquetCrs() needs to:

  1. inspect the crs object (e.g. check the id.authority/code fields to detect EPSG:4326/CRS84), and
  2. if it isn't a recognized lon/lat CRS, serialize the same object back to JSON.

With RapidJSON this was straightforward because the DOM is reusable, but with simdjson::ondemand the object has already been consumed by the time I want to serialize it, leading to OUT_OF_ORDER_ITERATION.

Is the expected approach here to restructure the code into a single pass, or is there another way to serialize an already-inspected ondemand::object?

Thanks!

@rok

rok commented Aug 3, 2026

Copy link
Copy Markdown
Member

As per docs here you could probably use the .reset() in case you don't get desired CRS fields.

  // Inspect crs_object...

  // If it was not a recognized lon/lat CRS:
  ARROW_ASSIGN_OR_RAISE(
      auto ignored,
      ::arrow::internal::GetSimdjsonResult(
          crs_object.reset(), "Failed to reset 'crs' object: "));

  ARROW_ASSIGN_OR_RAISE(
      auto raw_crs,
      ::arrow::internal::GetSimdjsonResult(
          crs_object.raw_json(), "Failed to get raw 'crs' JSON: "));

  return std::string(raw_crs);

Check also rewind and consider what's best for your usecase.

@Reranko05

Copy link
Copy Markdown
Collaborator Author

Thanks @rok, reset() + raw_json() fixed the iteration issue.

However, one test still fails because raw_json() preserves the original formatting (e.g. {"key0": "value0"}), whereas the previous RapidJSON Writer minified it to {"key0":"value0"}. LogicalType::Equals() compares the CRS string literally.

Would you prefer keeping the previous compact formatting or preserving the original JSON formatting?

@rok

rok commented Aug 3, 2026

Copy link
Copy Markdown
Member

@Reranko05 let's avoid behavior changes. Perhaps we can use simdjson::minify?

@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from 270af67 to 777999e Compare August 4, 2026 05:40
@Reranko05
Reranko05 marked this pull request as ready for review August 4, 2026 06:23
Copilot AI review requested due to automatic review settings August 4, 2026 06:23

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Reranko05

Reranko05 commented Aug 4, 2026

Copy link
Copy Markdown
Collaborator Author

@kou @rok @pitrou Could you review this when you have time?

@rok rok left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps we can remove rapidjson from here also?

Comment thread cpp/src/parquet/CMakeLists.txt Outdated
@rok

rok commented Aug 4, 2026

Copy link
Copy Markdown
Member

I also see many RapidJSON references in cpp/cmake_modules/ThirdpartyToolchain.cmake. Are we ready to remove those yet? (I don't know, hence the question)

@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Aug 4, 2026
Comment thread cpp/src/parquet/geospatial/util_json_internal.cc Outdated
Comment thread cpp/src/parquet/geospatial/util_json_internal.cc Outdated
Comment thread cpp/src/parquet/geospatial/util_json_internal.cc Outdated
Comment thread cpp/src/parquet/geospatial/util_json_internal.cc Outdated
Comment thread cpp/src/parquet/types.cc
@Reranko05

Copy link
Copy Markdown
Collaborator Author

I also see many RapidJSON references in cpp/cmake_modules/ThirdpartyToolchain.cmake. Are we ready to remove those yet? (I don't know, hence the question)

@rok RapidJSON is no longer used by Parquet after this change, so I've removed the Parquet-specific dependency. However, there are still many RapidJSON users elsewhere in Arrow (e.g. arrow/json, arrow/integration, arrow/extension, arrow/flight/sql), so I don't think we're ready to remove the ThirdpartyToolchain references yet.

Copilot AI review requested due to automatic review settings August 5, 2026 04:17
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from f0953e2 to 57bf182 Compare August 5, 2026 13:46

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings August 5, 2026 14:15
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from 57bf182 to 32d9dac Compare August 5, 2026 14:15

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings August 5, 2026 15:27
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from 32d9dac to 4121d9a Compare August 5, 2026 15:27

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from 4121d9a to 0fcef4d Compare August 5, 2026 16:13
@Reranko05
Reranko05 requested review from taepper and wgtmac August 5, 2026 16:38
@github-actions github-actions Bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Aug 7, 2026
@wgtmac

wgtmac commented Aug 7, 2026

Copy link
Copy Markdown
Member

https://github.com/apache/arrow/actions/runs/31024222689/job/92369046910?pr=50781

We need to fix meson ci due to ../../arrow/cpp/src/arrow/integration/meson.build:26:8: ERROR: Unknown variable "simdjson_dep".

Copilot AI review requested due to automatic review settings August 7, 2026 07:39

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions github-actions Bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Aug 7, 2026
Copilot AI review requested due to automatic review settings August 7, 2026 07:44
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from 0347ab3 to 3e16dbd Compare August 7, 2026 07:44

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings August 7, 2026 09:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings August 7, 2026 10:41
@Reranko05
Reranko05 force-pushed the gh-35460-pr5-updated branch from c16ef37 to e6b50cf Compare August 7, 2026 10:41

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@Reranko05

Copy link
Copy Markdown
Collaborator Author

@wgtmac Thanks for pointing it out, I have added the Meson support for simdjson and updated the relevant targets. The Meson CI is now passing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants